feat(lyrics-plus): change ♪ to idle dots #3455
Conversation
|
Run biome (2.0.0) on all files |
Done |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughIntroduces an InlineIdlingIndicator component and associated CSS. Refactors IdlingIndicator to render dots via a thresholds map. Updates SyncedLyricsPage to handle lines containing "♪" with a dedicated render path using InlineIdlingIndicator, retaining click-to-seek and existing CSS variable usage. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SyncedLyricsPage
participant InlineIdlingIndicator
participant IdlingIndicator
User->>SyncedLyricsPage: Render lyrics
alt Line text == "♪"
SyncedLyricsPage->>SyncedLyricsPage: Compute animationIndex, progress
SyncedLyricsPage->>InlineIdlingIndicator: Render with progress
User-->>SyncedLyricsPage: Click line (seek)
else Other lines
SyncedLyricsPage->>IdlingIndicator: Render (idle state)
User-->>SyncedLyricsPage: Click line (seek)
end
sequenceDiagram
participant Component as IdlingIndicator
participant Thresholds as [0.05,0.33,0.66]
Component->>Thresholds: Iterate thresholds
loop For each t
Component->>Component: Render dot active if progress ≥ t
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
CustomApps/lyrics-plus/Pages.js (1)
158-161: IdlingIndicator is always hidden here due to missing isActive prop.Class toggling uses
!isActive ? "lyrics-idling-indicator-hidden" : "". WithoutisActive, it’s undefined, so the indicator stays hidden.Apply:
- return react.createElement(IdlingIndicator, { - progress: position / activeLines[2].startTime, - delay: activeLines[2].startTime / 3, - }); + return react.createElement(IdlingIndicator, { + key: "intro-indicator", + isActive: true, + progress: position / activeLines[2].startTime, + delay: activeLines[2].startTime / 3, + });
🧹 Nitpick comments (3)
CustomApps/lyrics-plus/style.css (1)
763-771: Duplicate.lyrics-idling-indicatorselector overrides prior rules; also double-spacing risk.This block redefines
.lyrics-idling-indicator, which already exists above (Line 608). The later definition will override earlier values (display, transition). Additionally, the newgap: 0.4emcombined with the existing.lyrics-idling-indicator__circlemargin-right (Line 623) can produce doubled spacing between circles.
- Either remove
gaphere or drop the margin on circles for consistency.- Keep transition timing consistent with the earlier cubic-bezier easing, unless the change is intentional.
Apply this minimal diff to prevent double-spacing and keep easing consistent:
.lyrics-idling-indicator { - display: flex; - align-items: center; - gap: 0.4em; + display: flex; + align-items: center; + /* spacing provided by .lyrics-idling-indicator__circle margin-right above */ justify-content: center; margin: 0.5em 0; opacity: 0.7; - transition: opacity 0.2s; + transition: opacity 0.2s cubic-bezier(0, 0, 0.58, 1); }CustomApps/lyrics-plus/Pages.js (2)
34-37: Add stable keys when mapping dots to silence React warnings.Missing
keyon mapped elements can cause reconciliation warnings and subtle issues.Apply:
- [0.05, 0.33, 0.66].map((threshold) => - react.createElement("div", { className: `lyrics-idling-indicator__circle ${progress >= threshold ? "active" : ""}` }) - ) + [0.05, 0.33, 0.66].map((threshold, idx) => + react.createElement("div", { + key: `circle-${idx}`, + className: `lyrics-idling-indicator__circle ${progress >= threshold ? "active" : ""}`, + }) + )
40-53: Decorative inline dots should be hidden from assistive tech; consider a thresholds constant.
- Add
aria-hiddenso screen readers ignore purely decorative dots.- Optional: extract
[0.05, 0.33, 0.66]to a shared constant to avoid duplication.Apply:
return react.createElement( "span", { - className: `lyrics-inline-idling-indicator${isActive ? " active" : ""}`, + className: `lyrics-inline-idling-indicator${isActive ? " active" : ""}`, + "aria-hidden": "true", }, - [0.05, 0.33, 0.66].map((threshold, idx) => + [0.05, 0.33, 0.66].map((threshold, idx) => react.createElement("span", { key: idx, className: `lyrics-inline-idling-dot${progress >= threshold ? " active" : ""}`, }) ) );If you want to extract thresholds:
// near top of file const DOT_THRESHOLDS = [0.05, 0.33, 0.66];Then use
DOT_THRESHOLDS.map(...)in both indicators.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
CustomApps/lyrics-plus/Pages.js(2 hunks)CustomApps/lyrics-plus/style.css(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
CustomApps/lyrics-plus/Pages.js (3)
CustomApps/lyrics-plus/Utils.js (3)
react(170-170)react(174-174)i(179-179)Extensions/fullAppDisplay.js (3)
react(13-13)react(14-14)CONFIG(16-16)CustomApps/lyrics-plus/index.js (3)
react(6-6)react(7-7)CONFIG(30-96)
🔇 Additional comments (1)
CustomApps/lyrics-plus/style.css (1)
731-761: Inline idling indicator styles look good and scoped.The new
.lyrics-inline-idling-indicatorand.lyrics-inline-idling-dotclasses are self-contained, use sensible fallbacks for CSS vars, and transition properties are appropriate for a subtle effect. Vertical alignment and spacing read nicely.
| .lyrics-idling-dot { | ||
| width: 0.7em; | ||
| height: 0.7em; | ||
| border-radius: 50%; | ||
| background: var(--lyrics-idling-dot-color, #888); | ||
| opacity: 0.3; | ||
| transform: scale(0.7); | ||
| transition: | ||
| background 0.2s, | ||
| opacity 0.2s, | ||
| transform 0.2s; | ||
| display: inline-block; | ||
| } | ||
| .lyrics-idling-dot.active { | ||
| background: var(--lyrics-idling-dot-active-color, #fff); | ||
| opacity: 1; | ||
| transform: scale(1.1); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify usage of new .lyrics-idling-dot selectors.
Pages.js keeps using lyrics-idling-indicator__circle for the block indicator. If .lyrics-idling-dot isn’t referenced elsewhere, these rules are currently unused.
Run this to confirm usage and remove dead CSS if unused:
🏁 Script executed:
#!/bin/bash
# Find references to the new dot classes
rg -n -C2 'lyrics-idling-dot|lyrics-inline-idling-dot'Length of output: 2162
🏁 Script executed:
#!/bin/bash
# Check for usage of the old block indicator class
rg -n -C2 'lyrics-idling-indicator__circle'Length of output: 979
Remove or Integrate Unused .lyrics-idling-dot Styles
Our search confirms that:
.lyrics-inline-idling-dotis used in CustomApps/lyrics-plus/Pages.js (line 49)..lyrics-idling-dot(defined in style.css lines 775–792) isn’t referenced anywhere in the codebase.- The block indicator in Pages.js (line 35) still uses
lyrics-idling-indicator__circle.
Action items:
- If you intended to replace the block‐indicator dots with
.lyrics-idling-dot, update thePages.jsrender logic:- className={`lyrics-idling-indicator__circle ${progress >= threshold ? "active" : ""}`} + className={`lyrics-idling-dot ${progress >= threshold ? "active" : ""}`}
- Otherwise, remove the unused
.lyrics-idling-dotrules from CustomApps/lyrics-plus/style.css (lines 775–792).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .lyrics-idling-dot { | |
| width: 0.7em; | |
| height: 0.7em; | |
| border-radius: 50%; | |
| background: var(--lyrics-idling-dot-color, #888); | |
| opacity: 0.3; | |
| transform: scale(0.7); | |
| transition: | |
| background 0.2s, | |
| opacity 0.2s, | |
| transform 0.2s; | |
| display: inline-block; | |
| } | |
| .lyrics-idling-dot.active { | |
| background: var(--lyrics-idling-dot-active-color, #fff); | |
| opacity: 1; | |
| transform: scale(1.1); | |
| } | |
| className={`lyrics-idling-dot ${progress >= threshold ? "active" : ""}`} |
🤖 Prompt for AI Agents
In CustomApps/lyrics-plus/style.css around lines 775 to 792 the
.lyrics-idling-dot rules are defined but not referenced anywhere; either delete
this unused CSS block, or update CustomApps/lyrics-plus/Pages.js (check the
block indicator at ~line 35 and the inline usage at ~line 49) to replace the
existing indicator classes (e.g., lyrics-idling-indicator__circle or
.lyrics-inline-idling-dot) with .lyrics-idling-dot and ensure you add the
.active class where the active state is toggled and the CSS custom properties
(--lyrics-idling-dot-color, --lyrics-idling-dot-active-color) are set or fall
back to desired defaults.
|
@Mytic2330 please do what coderabbit says. if it's wrong (hallucinated new lines or smth) then ignore it but if it did not, please apply the suggestion(s) |
2 participants
This pull pull request is to change the lyrics display functionality in the
CustomApps/lyrics-plus. The new thing added is an inline idling indicator as suggested in #3363. It changes all ♪ in the lyrics to times dots as at the intro of the song.Summary by CodeRabbit